This repository was archived by the owner on Apr 14, 2022. It is now read-only.
Repository navigation
Improve diagnostic generation/handling, enable diagnostics with setting - #289
Closed
Jake Bailey (jakebailey) wants to merge 4 commits into
Closed
Jake Bailey (jakebailey) wants to merge 4 commits into
Jake Bailey (jakebailey) wants to merge 4 commits into
Conversation
Jake Bailey (jakebailey)
requested review from
Alexander Sher (AlexanderSher) and
Mikhail Arkhipov (MikhailArkhipov)
October 19, 2018 21:22
Member
Author
|
Of course, the added setting needs to make it into VSC before their release, otherwise #242 isn't fixed in principle. |
| var reanalyze = true; | ||
| if (@params.settings != null) { | ||
| if (@params.settings is ServerSettings settings) { | ||
| Analyzer.EnableDiagnostics = settings.analysis.diagnostics; |
There was a problem hiding this comment.
The setting needs to be added to the package.json in the extension defaulting to true
Member
Author
There was a problem hiding this comment.
Yep, I've done that locally for now (until I PR it), since I wasn't 100% sure on the naming.
There was a problem hiding this comment.
Don Jayamanne (@DonJayamanne) can suggest name of the setting
This was referenced Oct 22, 2018
Member
Author
|
I'm going to close this in favor of #298. Adding this setting complicates things in terms of how to deal with parsing diagnostics that I'm not confident this change can properly handle. |
Jake Bailey (jakebailey)
added a commit
that referenced
this pull request
Oct 23, 2018
Fixes #242. #289 causes a few issues to do with parser-generated diagnostics, and there are more diagnostics I wasn't aware of coming from ErrorSink. This is a simplified PR that does the minimum to enable diagnostics (ignoring "Unspecified" at publish time) and fixes the setting change bug. We'll have to revisit where diagnostics should be stored to access via code actions in a different PR. There are only two diagnostics that actually come out of the server which can be disabled with `python.analysis.disabled` for now. I'll close #289 in favor of this one.
Jake Bailey (jakebailey)
added a commit
to jakebailey/python-language-server
that referenced
this pull request
Nov 1, 2019
) Fixes microsoft#242. microsoft#289 causes a few issues to do with parser-generated diagnostics, and there are more diagnostics I wasn't aware of coming from ErrorSink. This is a simplified PR that does the minimum to enable diagnostics (ignoring "Unspecified" at publish time) and fixes the setting change bug. We'll have to revisit where diagnostics should be stored to access via code actions in a different PR. There are only two diagnostics that actually come out of the server which can be disabled with `python.analysis.disabled` for now. I'll close microsoft#289 in favor of this one.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #242.
This does the following:
codetoseveritywas updated, not the actual listing, so diagnostics were never re-run even when settings were changed.python.analysis.diagnosticswhich is true when diagnostics should be published altogether (and also triggers an update when changed).The default behavior remains the same; users will not see any diagnostics until VSC adds the setting to itsDefault will be enabled, as per PR comments.package.json. Devs (us) can set this in the JSON just like we setdownloadLanguageServereven though it's undocumented.Looking for feedback on:
python.analysis.diagnostics.EnableDiagnosticsnow that the toggle is focused on deciding whether or not to publish diagnostics, not whether or not to collect them.